Changes to become v1.x by aredridel · Pull Request #30 · krakenjs/engine-munger

aredridel · 2015-06-01T15:56:26Z

No description provided.

…ing systems

…y of the engine it modifies

This exposes the (potential) express 5 style asynchronous locate, and by blessing the view.locate() function, engines can use this to look up resources.

since i18n for a dust engine makes no sense as Kraken uses it, this test can be much simpler.

grawk · 2015-06-01T23:56:30Z

I guess it looks like there is no need to check that err is defined here..

Yeah. I'm not super happy with that logic -- it works, it's correct, but it's not obvious.

grawk · 2015-06-02T18:07:33Z

I had to run npm run-script cover. Is this due to my npm version? (2.5.1)

No, run has always been an alias for run-script. What happened when you did npm run cover?

oh. sorry. i was simply using npm cover and then straight to npm run-script cover.. I didn't properly read the doc above and try npm run. That works for me now that I've tried it.

Awesome coverage metrics btw!!

grawk · 2015-06-02T19:24:27Z

I think this needs to be .nyc_output/

grawk · 2015-06-02T19:35:54Z

Is there a sample express app anywhere currently using the v1.x pre-release version of engine-munger?

aredridel · 2015-06-02T19:40:35Z

Working on integrating it into a kraken app right now.

aredridel · 2015-06-02T19:40:44Z

Also, the tests fire up a complete app.

…onomics of lookup

grawk · 2015-06-02T21:57:58Z

Ah right. I guess the root of my question was really "which versions of adaro and other things go along with this?" but it's really right there in package.json already! Aside from the picky stuff I already noted I don't have any other concerns. But let me digest this some more to make sure I can learn as much as possible before giving a thumbs up.

aredridel · 2015-06-02T22:02:08Z

awesome. It's really the design and redrawing of module boundaries I want to make sure make sense.

grawk · 2015-06-02T23:01:37Z

In terms of structure, what do you think about externalizing the proto patch from the exported makeViewClass function? I'm just thinking in terms of the fact that the patch will hopefully go away when the PR is accepted to express and this module is released to match express@5.x.

edit: Let me restate this: would there be sufficient value to include the express view patch verbatim in this module, and then externalize all the "non-patch" stuff (like bcp47/i18n resolution)? That could make more clear how we will eventually utilize express once 5.x is released.

grawk · 2015-06-02T23:22:13Z

freshy isn't used in this module

aredridel · 2015-06-03T03:13:04Z

Mostly the prototype creation is inside makeViewClass because the conf variable is shared with the internals but private.

None of that is performance critical, since a new view class is created once per app in normal use, so having it be properly private by scoping made me happy.

grawk · 2015-06-03T15:42:14Z

Ok. Officially in love with this PR. 👍

pvenkatakrishnan · 2015-06-03T19:24:56Z

👍

Changes to become v1.x

aredridel added 29 commits June 1, 2015 11:56

Directly depend on adaro, and let it bring in dustjs

c50c6f9

Update built fixtures to use dust 2.7 format

62624e3

Pass specialization via requestOptions

68aae66

Fix specialization test and test both paths

0b18d7d

Simplify and make unconditional the onLoad wrappers

400efff

Syntactic cleanups

1b92a95

Use dustjs.loadSource and return template instead of source

0f440b1

Rework test suite

efa64d0

Check all i18n and specialization combinations in spclAndIntl-js test

71e879a

Add option enableMetadata to allow hooking by in-place content edit…

e30dfea

…ing systems

Appease the linter

6a8d836

Improve package.json metadata

a35d02b

Rework expressView to mimic the original better and work independentl…

5e125f7

…y of the engine it modifies

Switch from tape to tap

312c099

Add nyc for coverage tracking in tests

055be18

Replace expressView with one with async lookup

8b666b5

This exposes the (potential) express 5 style asynchronous locate, and by blessing the view.locate() function, engines can use this to look up resources.

Make karka unconditional

491847b

Rework to use permutron

c34d1d1

Implement i18n lookups cleanly

fa385dd

Remove dead code

036e3b0

Update tests to use expressView, rather than the engine directly

c0ec0b6

Make adaro a dev dependency

eb6b5cb

Update the README

11dc7f0

Remove dead code

5657129

Make error message for not found template slightly more useful

082ea49

Stop checking for i18n on invalid parse check

d41cfab

since i18n for a dust engine makes no sense as Kraken uses it, this test can be much simpler.

Check new, better error message

422073f

simplify lookupMain and test it

5c5cb94

Test having multiple roots to search

f224899

aredridel force-pushed the next branch from 6f7399a to 6707f05 Compare June 1, 2015 16:04

aredridel added 3 commits June 1, 2015 14:55

Add myself as a contributor

528b2c1

Remove middleware, moved to makara

7c77448

Remove direct devDependency on istanbul, we have nyc for that

206295a

aredridel force-pushed the next branch from 88b52cd to 206295a Compare June 1, 2015 19:31

grawk reviewed Jun 1, 2015
View reviewed changes

Add reference to Express 5 change

7e8422c

grawk reviewed Jun 2, 2015
View reviewed changes

Expose regnerate-test-data script

c600fda

aredridel force-pushed the next branch from 9ce132f to c600fda Compare June 2, 2015 19:10

grawk reviewed Jun 2, 2015
View reviewed changes

Improve .gitignore

457e310

aredridel added 3 commits June 2, 2015 16:15

Move error handling into lookup rather than lookupMain to improve erg…

9c8b299

…onomics of lookup

Bump adaro to get version that always passes the extension to lookup

5af265a

1.0.0-3

6f99310

grawk reviewed Jun 2, 2015
View reviewed changes

Remove useless freshy dependency

0a7d7c2

pvenkatakrishnan pushed a commit that referenced this pull request Jun 3, 2015

Merge pull request #30 from krakenjs/next

bbcd258

Changes to become v1.x

pvenkatakrishnan merged commit bbcd258 into v1.x Jun 3, 2015

aredridel deleted the next branch June 3, 2015 19:52

Uh oh!

Conversation

aredridel commented Jun 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grawk commented Jun 2, 2015

Uh oh!

aredridel commented Jun 2, 2015

Uh oh!

aredridel commented Jun 2, 2015

Uh oh!

grawk commented Jun 2, 2015

Uh oh!

aredridel commented Jun 2, 2015

Uh oh!

grawk commented Jun 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aredridel commented Jun 3, 2015

Uh oh!

grawk commented Jun 3, 2015

Uh oh!

pvenkatakrishnan commented Jun 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants